Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Library: add control to open/close the track menu #4465

Merged
merged 2 commits into from
Mar 16, 2022

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Oct 21, 2021

Note: I don't intend to merge this, unless others consider it a useful addition, of course.
This is just to get a review if someone is interested.

This adds [Library],show_track_menu control. It's read/write, i.e. set to 1 to open the track menu for the track currently selected in the tracks table at the current mouse pointer position -- set to 0 to close it.
It's set to 1 if there's an open track context menu, no matter if opened with right-click, menu key (Shift+F10?) or the CO.
This is only supposed to work with the track menu of the tracks table -- other instances opened from the deck labels are not registered. The tracks table does not need to be focused for this to work.

In conjunction with #4618 this allows me to pick a query from the "Search related tracks" submenu easily with encoder pushs (Enter) and turns (Up / Down), so I need to use my laptop's touchpad even rarer.

@JoergAtGithub
Copy link
Member

This CO makes sense to me. Especially with the Search Related Track function.

@ronso0
Copy link
Member Author

ronso0 commented Dec 20, 2021

Yeah, and it works well. It's also useful for deleting a track from disk.
I've mapped it to Shift + Trax knob press and the navigation works well: turn scroll, press confirm.

There's still an issue with submenus: in ~10% of the attmpts I can open for example the "Search related" menu but for some reason subsequent move/scroll commands are still sent to the main tracks menu. Seems to be a focus issue related to the cursor position because that determines in which direction the menu unfolds.
The current implementation may work better, though I didn't test it yet with a controller. Diff: before I used only the cursor pos, now I use the existing contextMenuEvent slot with an emulated QContextMenuEvent incl. QContextMenuEvent::Mouse reason.

Some more findings from real life testing:

  • if a dialog has focus [Library], Move... controls should sent Tab / Shift+Tab keystrokes (not up/down/left/right)
  • if a dialog has focus [Library], GoToItem control should sent Space instead of Enter

@daschuer
Copy link
Member

daschuer commented Dec 22, 2021

I can confirm that this is useful. Especially when we think of creating a touch skin.

@ronso0
Copy link
Member Author

ronso0 commented Dec 26, 2021

Okay, there's interest so this could be merged?
I'll test the updated commit with my controller to see if it works 100% for submenus.

@ronso0 ronso0 marked this pull request as ready for review January 15, 2022 23:45
@ronso0
Copy link
Member Author

ronso0 commented Jan 15, 2022

Works nicely! Tested with #4618

@daschuer
Copy link
Member

Can we close this PR now since it is part of #4618

@ronso0
Copy link
Member Author

ronso0 commented Jan 16, 2022

No, I prefer to have those feature in separate PRs.
It's in #4618 only to simplify testing the new menu navigation. I'll rebase that once this PR is merged.

@ronso0
Copy link
Member Author

ronso0 commented Jan 16, 2022

I changed the control to [Library],toggle_track_menu

@JoergAtGithub
Copy link
Member

Is toggle really what suits best? If we want to toggle from a script, we've allready script.toggleControl
But can I map an ESC button, which should only close, but never open a menu?

@ronso0
Copy link
Member Author

ronso0 commented Jan 16, 2022

The difference is that this does not toggle the control but the context menu, only 1 is processed: you'd always use engine.setvalue("[Library],toggle_track_menu", 1); and the c++ code invokes the appropriate action.
This works fine with controllers, as well as with a future GUI / touch screen implementation where we only need a 1-state trigger button (IMO there's no point in having that button indicate the menu visibility, as well as no point in using it to close the menu as you would simply click anywhere outside the menu, as usually).

@daschuer
Copy link
Member

For me the new name is clear, but I don't mind to change back to the old name, if it is considered to be confusing. Just give me a hint when this is ready to be merged.

@Swiftb0y
Copy link
Member

IMO I prefer the richer CO where you can actually read the state from the CO. I do not see the benefit of hiding that information via a "smarter" toggle CO.

@ronso0
Copy link
Member Author

ronso0 commented Jan 16, 2022

This toggle is only for opening/closing WTrackMenu parented to WTrackTableView. The connection is made in LibraryControl > WTrackTableView (> m_pTrackMenu). It's not "smart", it's simple and efficient.
For a read/write CO an aditional connections are required, for example overriding QMenu::closeEvent and sending that all the way to LibraryControl...
Is it really worth the effort for a rather obvious widget?
What is the benefit of reading the visible state?

FWIW if the visible state is intended for elaborate scripting, this is already possible with #4618 which extends the [Library],focusedWidget enum by 4 ContextMenu(all context menus and those of the main menu bar) and 5 Dialogs.

@Swiftb0y
Copy link
Member

FWIW if the visible state is intended for elaborate scripting, this is already possible with #4618 which extends the [Library],focusedWidget enum by 4 ContextMenu(all context menus and those of the main menu bar) and 5 Dialogs.

So is it possible to open these menus by setting focusedWidget to the appropriate value?
I'm worried that its a bad design for a CO that has overloaded capability (close menu when visible, open context menu when not). This seems to me like a similar mistake in design where beatjump CO would also move the loop with it when the loop was active. Lots of people didn't want that so they worked around that in their mappings. Thats why I'm saying that this new CO is "smart". Also, being a "write-only" CO makes it harder to use because its essentially opaque. Having to query another CO (focusedWidget) to be able to reason about the behavior of toggle_track_menu is an awkward API.

@ronso0
Copy link
Member Author

ronso0 commented Jan 17, 2022

So is it possible to open these menus by setting focusedWidget to the appropriate value?

no, only existing library widget can be focused with [Library],focused_widget (enum 1-3).
The other values (4, 5, 6, 0) are read-only: any QMenu, any QDialog, unvategorited widgets, none

Reverted the name to [Library],show_track_menu
And I made the track menu toggle read/writeable, went quicker than I thought. Still: this works only for the WTrakMenu of the WTrackTableViews! It will not be aware of other WTrackMenu instances, so multiple menus can be open (though only the most recent one can receive keypresses, GoToItem and MoveUp/Down/Vertical, with #4618)

@Swiftb0y
Copy link
Member

this works only for the WTrakMenu of the WTrackTableViews

Thank you. I think this is fine as long as we document it as part of the MixxxControls.

@ronso0
Copy link
Member Author

ronso0 commented Jan 17, 2022

Sure, I'll update the manual after merge.

@ronso0
Copy link
Member Author

ronso0 commented Jan 17, 2022

Documentation comments updated, rebased, squashed, ready for review!

@ronso0
Copy link
Member Author

ronso0 commented Jan 17, 2022

(forgot to git add some comments)

@ronso0 ronso0 added this to the 2.4.0 milestone Jan 26, 2022
@daschuer
Copy link
Member

This is mergeable now. However some conflicts have been developed.

@ronso0
Copy link
Member Author

ronso0 commented Feb 28, 2022

I'll take care of it in two weeks.

@ronso0
Copy link
Member Author

ronso0 commented Mar 16, 2022

This is mergeable now. However some conflicts have been developed.

Fixed, ready for merge

@daschuer
Copy link
Member

Thank you!

@daschuer daschuer merged commit c8107cb into mixxxdj:main Mar 16, 2022
@ronso0 ronso0 deleted the open-track-menu-control branch March 16, 2022 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants